fix(Ray): Retain the original function name when patching Ray tasks #4858
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![seer-by-sentry[bot]](https://avatars.githubusercontent.com/in/801464?s=60&v=4){kind=small}
| signature = inspect.signature(new_func) | ||
| params = list(signature.parameters.values()) | ||
| params.append( | ||
| inspect.Parameter( | ||
| "_tracing", | ||
| kind=inspect.Parameter.KEYWORD_ONLY, | ||
| default=None, | ||
| ) | ||
| ) | ||
| new_func.__signature__ = signature.replace(parameters=params) | ||
|
|
There was a problem hiding this comment.
Potential bug: Adding the _tracing parameter without checking for its existence can cause a ValueError if the user's function already has a parameter with that name, crashing the application.
-
Description: The Ray integration modifies the signature of a user's function to add a
_tracingparameter. However, it does not first check if a parameter with that name already exists. If a user decorates a function that already has a_tracingparameter, the code attempts to add a duplicate. This will causeinspect.signature.replace()to raise aValueErrorbecause duplicate parameter names are not allowed. This error is unhandled and will crash the application when the function is decorated, preventing the application from starting. -
Suggested fix: Before appending the
_tracingparameter to theparamslist, iterate through the existing parameters to check if one with the name_tracingalready exists. If it does, do not append the new parameter.
severity: 0.7, confidence: 0.95
Did we get this right? 👍 / 👎 to inform future reviews.
|
Follow-up to the CI run:
|
There was a problem hiding this comment.
Hey @svartalf, thanks for the PR! TL;DR, it all looks good to me, but see below for a couple of optional suggestions.
Re: LLM suggestions, we could rename the arg to _sentry_tracing or something less generic to reduce the likelihood of collisions. FWIW I don't think it's necessary, so feel free to ignore.
If you can add a test or integrate a check for this into one of the tests in the existing test suite, that'd be awesome.
Approving already though since it looks good to me as is, pending the type: ignore.
Without "@functools.wraps" added, Ray exposes Prometheus metrics with all tasks named "new_func"
svartalf
force-pushed
the
fix/ray-new-func-name
branch
from
0b68160 to
93ce310
Compare
|
Thanks for the review! I added all suggestions, CI is green now :) |
|
TY! |
Description
Without "@functools.wraps" added, Ray exposes Prometheus metrics with all tasks named "new_func"
Issues
Reminders
tox -e linters.feat:,fix:,ref:,meta:)